-
Notifications
You must be signed in to change notification settings - Fork 270
[DOCS] Rest 5 client fixes #1047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔍 Preview links for changed docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Java Rest 5 client documentation to fix stale details and improve clarity. The changes address outdated API references, improve explanations of timeout configurations, and enhance authentication documentation.
- Updates timeout configuration examples to use current API methods and clearer explanations
- Improves basic authentication documentation with better descriptions and current API references
- Adds cross-references and enhances formatting for better readability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
docs/reference/transport/rest5-client/config/timeouts.md | Updates timeout configuration examples and adds detailed explanations of different timeout types |
docs/reference/transport/rest5-client/config/basic_authentication.md | Improves basic authentication documentation with clearer explanations and updated API references |
|
||
% :::{include-code} src={{doc-tests-src}}/rest5_client/RestClientDocumentation.java tag=rest-client-config-timeouts | ||
```java | ||
Rest5ClientBuilder builder = Rest5Client | ||
.builder(new HttpHost("localhost", 9200)) | ||
.builder(new HttpHost("http", "localhost", 9200)) <1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor arguments are in the wrong order. According to the HttpHost constructor, the correct order should be new HttpHost("localhost", 9200, "http")
where hostname comes first, then port, then scheme.
.builder(new HttpHost("http", "localhost", 9200)) <1> | |
.builder(new HttpHost("localhost", 9200, "http")) <1> |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l-trotta I'm not sure Copilot is right about this. On this apache page, it says "For constructors that take a scheme as an argument, that argument is now the first one."
Should http
be first or last?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah my bad for mixing it up in the example, copilot is still wrong though ^^" correct order is: scheme, host, port. so it should be .builder(new HttpHost("http","localhost", 9200))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
http
be first or last?
I went ahead and put it first, but happy to revert
@l-trotta please check this for accuracy! ty and no rush |
Update stale details in the Java Rest 5 client docs (see linked issues). Also did a general edit for clarity, style, etc.
Closes
elastic/docs-content#2120 and
elastic/docs-content#2121
preview links
docs/reference/transport/rest5-client/config/basic_authentication.md
docs/reference/transport/rest5-client/config/timeouts.md